-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ContainerRegistry] switch to use official TS code generator #15777
Conversation
and re-generate code.
? `${options.userAgentOptions.userAgentPrefix} ${packageDetails}` | ||
: `${packageDetails}`; | ||
options.userAgentOptions = { | ||
userAgentPrefix: userAgentPrefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were we setting the user agent string in the convenience layer before? If so, are we now ending up with duplicates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. This would lead to duplicate of azsdk-js-container-registry/1.0.0-beta.4
. In convenience layer we already append library info to options.userAgentOptions.userAgentPrefix
.
@sarangan12 what are your thoughts? Update code gen to not append packageDetails when userAgentPrefix is passed via options, or update convenice layer to not append library info?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were thinking of convenience layer not appending library info. From the very beginning this was one of the things that we felt could be generated. Now that we are generating, we should make use of that :)
The only thing that should be added as a prefix to this is what the user provides
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I will update the convenience layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a bug in the codegen: my code is creating two generated clients using the same options. However, as the generated code mutates my option object, I end up having duplicate library info. I fixed it by making an updated copy of the options bag.
- options.userAgentOptions = {
- userAgentPrefix: userAgentPrefix
- };
-
const optionsWithDefaults = {
...defaults,
...options,
+ userAgentOptions: {
+ userAgentPrefix
+ },
baseUri: options.endpoint || "{url}"
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we use the same swagger file as before or were there changes to the swagger file too? Am wondering if it would be easier to review the PR if we used the same swagger file with the new code generator. Then we can attribute all the changes to the code generator and not the swagger changes |
Yes same swagger. The only manual change I made is switching from using my local autorest.typescript to the released package. |
sdk/containerregistry/container-registry/src/generated/generatedClientContext.ts
Show resolved
Hide resolved
Lets go! |
{ | ||
"path": "src/constants.ts", | ||
"prefix": "SDK_VERSION" | ||
"prefix": "const packageDetails" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremymeng do you remember why having the const
part here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deyaaeldeen
Because this metadata is named "prefix" and the line
const packageDetails =
azsdk-js-container-registry/1.0.0-beta.4;
contains const packageDetails
at the beginning. However it doesn't have to be here, if packageDetails
alone also works when the bot is bumping versions.
and re-generate code.
The only manual change is in swagger\README.md. Rest are from generated code.